-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix reporting when skipping extensions #3254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
The lists ext and ext_instances become out of sync when extensions are skipped. Hence use ext_instances as the true source for reporting Also fix the documentation of skip_extensions and add verbose reporting to stdout
c55a115
to
cd49c19
Compare
Changes look good, but I'm wondering if we can make sure the tests trip over the misreporting? |
Not without actually running a real build and intercepting the output. Sounds like a bit of overkill for me. |
@Flamefire We try to get all bug fixes covered by the tests, to avoid re-introducing the issue later. Done in Flamefire#1 |
Ah that easy, thanks! |
minor fix + enhanced test for skipped extensions
@Flamefire I think the issue was that the Travis GitHub App was not registered yet, which I've fixed just now. We shouldn't be hitting rate limits at all I think... |
60/hour isn't much. In the mentioned PR I use an endpoint that isn't even rate limitted hence should work better. |
The lists ext and ext_instances become out of sync when extensions are skipped.
Hence use ext_instances as the true source for reporting
Also fix the documentation of skip_extensions and add verbose reporting to stdout
Fixes #3253